Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: modularize checksums #1568

Merged
merged 42 commits into from
Jun 19, 2024
Merged

feat: modularize checksums #1568

merged 42 commits into from
Jun 19, 2024

Conversation

dayaffe
Copy link
Contributor

@dayaffe dayaffe commented Jun 13, 2024

New modularization package:

  • AWSSDKChecksums

Issue #

#1532

Description of changes

  • AWSSDKChecksums contains details specific to the AWS part of checksums. As of now this includes the method to add the aws-chunked header. AWSSDKChecksums can only contain checksum and chunking related methods that are specific to AWS and are not dependencies of other parts of the smithy-swift package.
  • Adds additional dependencies from smithy-swift including SmithyChecksums and SmithyStreams
  • Protocol tests now depend on SmithyStreams and a symbol has been added for SmithyStreams.BufferedStream.
  • FlexibleChecksums Request/Response middlewares have been moved from smithy-swift to aws-sdk-swift

New/existing dependencies impact assessment, if applicable

Conventional Commits

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dayaffe dayaffe requested review from jbelkins and sichanyoo June 17, 2024 15:34
@@ -255,3 +256,14 @@ public class AWSSigV4Signer: SmithyHTTPAuthAPI.Signer {
}
}
}

extension SigningConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method doesnt rely on checksums

@@ -30,6 +30,7 @@ import struct SmithyHTTPAuthAPI.SigningFlags
import struct Foundation.Date
import struct Foundation.TimeInterval
import struct Foundation.URL
import AWSSDKChecksums
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the signer still needs to depend on AWSSDKChecksums for setChunkedBody method

Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question on need for AWSSDKChecksums module

@@ -22,6 +22,7 @@ extension Target.Dependency {
static var awsSDKEventStreamsAuth: Self { "AWSSDKEventStreamsAuth" }
static var awsSDKHTTPAuth: Self { "AWSSDKHTTPAuth" }
static var awsSDKIdentity: Self { "AWSSDKIdentity" }
static var awsSDKChecksums: Self { "AWSSDKChecksums" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is a separate module actually necessary for just a single extension with two methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish more could be moved to AWSSDKChecksums but that's dependent on CRT HTTP Client being refactored in the future. I'd say even if its only two methods its good to have but I don't feel strongly. aws-chunked is pretty explicitly aws-only though and one of those methods adds it to the headers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. We could keep it the way it is in this PR, or move it into AWSClientRuntime. For both cases we would revisit this down the line. I don't feel strongly about either option. Unless Josh has preference for one over the other I suppose we can keep AWSSDKChecksums.

@dayaffe dayaffe requested a review from sichanyoo June 19, 2024 14:55
@dayaffe dayaffe merged commit daabd8f into main Jun 19, 2024
29 checks passed
@dayaffe dayaffe deleted the day/modular-checksums branch June 19, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants